Skip to content

DataHandle: findString(..) add support handles with unknown length#339

Merged
ctrueden merged 2 commits intomasterfrom
improve-find-string
Oct 16, 2018
Merged

DataHandle: findString(..) add support handles with unknown length#339
ctrueden merged 2 commits intomasterfrom
improve-find-string

Conversation

@gab1one
Copy link
Contributor

@gab1one gab1one commented Oct 15, 2018

  • The findString(..) method used to require the exact length of the
    handle, this length can be unknown, e.g. for compressed handles.
    This is no longer the case

@gab1one gab1one requested a review from ctrueden October 15, 2018 13:48
@gab1one gab1one mentioned this pull request Oct 15, 2018
Closed
ctrueden
ctrueden reviewed Oct 15, 2018
View reviewed changes
Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of quick questions:

  • How much effort would be required to add a test? If easy, please do it.
  • Did you verify same performance? From the patch it does not appear there would be an issue, but good to be certain.

@gab1one gab1one changed the title DataHandle: fix findString(..) and support handles with unknown length DataHandle: findString(..) add support handles with unknown length Oct 15, 2018
@gab1one
Copy link
Contributor Author

gab1one commented Oct 15, 2018

@ctrueden regarding the testing, I encountered this bug when working on #274, there I was able to test it as well.

@gab1one gab1one force-pushed the improve-find-string branch from f0780ac to 6cf9dd9 Compare October 15, 2018 15:06
DataHandle: fix findString(..) and support handles with unknown length
4a365aa 
The findString(..) method used to require the exact length of the
handle, this length can be unknown, e.g. for compressed handles.
This is	no longer the case.
@gab1one gab1one force-pushed the improve-find-string branch from 6cf9dd9 to 4a365aa Compare October 15, 2018 15:11
@gab1one
Copy link
Contributor Author

gab1one commented Oct 15, 2018

I just cleaned the code up a bit more, now it should be more clear and length() is not used anywhere.

@ctrueden
Copy link
Member

ctrueden commented Oct 15, 2018

Thanks @gab1one, but my previous two questions still stand:

  1. Would it be possible to add a regression test for findString, which failed previously, but passes now?
  2. Did you do any performance checking?

@gab1one
Copy link
Contributor Author

gab1one commented Oct 15, 2018 •
edited
Loading

  1. I can try building such a test, it would require a handle that reports it's length wrongly.
  2. I did not, but I did not change how the actual processing logic work, basically I just moved the reading into the while loop head and used the number of read bytes as a EOF signal instead of the length reported by the handle. I can try to do some performance testing together with 1).

@ctrueden
Copy link
Member

ctrueden commented Oct 15, 2018

it would require a handle that reports it's length wrongly.

Why? I thought the issue was for handles that do not know their own length. As in: they return -1? Did I misunderstand?

I did not change how the actual processing logic work

Fair enough; let's not worry about it then.

@gab1one
Copy link
Contributor Author

gab1one commented Oct 16, 2018

Yes ones that return -1. I added a test that reproduces the bug and discovered another one :), BytesHandle did not return -1 on EOF, see #341

@ctrueden ctrueden merged commit a922e15 into master Oct 16, 2018
@ctrueden ctrueden deleted the improve-find-string branch October 16, 2018 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ctrueden ctrueden ctrueden left review comments

Assignees

@gab1one gab1one

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gab1one @ctrueden